Add `apply_block_events` and `apply_block_connected_to_events` #336

tnull · 2025-10-29T12:18:43Z

Description

Previously, we added a new Wallet::apply_update_events method that returned WalletEvents. Unfortunately, no corresponding APIs were added for the apply_block counterparts. Here we fix this omission.

Notes to the reviewers

I opened this towards the release-2.2 branch, but it would probably need another release branch. Or let me know if you prefer to open it against master (which seems to be lacking apply_update_events currently though).

I also added no test coverage given that none seems to exist for Wallet::apply_block in the first place. Let me know if I should add something here.

Checklists

All Submissions:

I've signed all my commits
I followed the contribution guidelines
I ran just p before pushing

New Features:

I've added tests for the new feature
I've added docs for the new feature

cc @notmandatory

coveralls · 2025-10-29T12:23:27Z

Pull Request Test Coverage Report for Build 18988009424

Details

34 of 69 (49.28%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.03%) to 85.079%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
wallet/src/wallet/mod.rs	34	69	49.28%

Totals
Change from base Build 18951786142:	0.03%
Covered Lines:	7059
Relevant Lines:	8297

💛 - Coveralls

Camillarhi · 2025-10-29T18:18:55Z

wallet/src/wallet/mod.rs

+        block: &Block,
+        height: u32,
+    ) -> Result<Vec<WalletEvent>, CannotConnectError> {
+        let connected_to = match height.checked_sub(1) {


Thanks for working on this. I noticed apply_block_events seems to duplicate logic from apply_block. Could we move the event handling from apply_block_connected_to_events into apply_block_events, then have it call apply_block instead of apply_block_connected_to? Would be more consistent with how apply_update_event works and avoid the duplication.

No, I intentionally added variants for apply_block as well as for apply_block_connected_to as we may also want to use apply_block_connected_to_events at some point.

You could have apply_block call the new apply_block_events and map the return value to ().

You could have apply_block call the new apply_block_events and map the return value to ().

Hmm, but that would run the (possibly costly) delta calculation for everybody, even if they wouldn't make use of the events. I believe this is why @notmandatory added separate _event variants of the methods in the first place.

In #319 (in v3.0) my plan is to always return events. But for this PR I agree it's best keep the two paths separate so events are only calculated on the _events functions.

notmandatory · 2025-10-30T00:13:39Z

PR needs a rebase on the release/2.2 branch to fix the CI issue, see #338.

Previously, we added a new `Wallet::apply_update_events` method that returned `WalletEvent`s. Unfortunately, no corresponding APIs were added for the `apply_block` counterparts. Here we fix this omission.

tnull · 2025-10-30T09:37:52Z

PR needs a rebase on the release/2.2 branch to fix the CI issue, see #338.

Rebased on release/2.2, but still wondering if we'd need a release/2.3 branch for this, given it extends API?

ValuedMammal · 2025-10-30T19:00:51Z

ACK I changed the PR to target release/2.3. I agree having a unit test would be nice. I assume we'll need a similar update to #319 .

notmandatory

Thanks for doing this. Other than my suggested change to apply_wallet_events() it looks good to me.

I would like to see some tests. If you don't have time to do them I'm happy to push a commit with similar testing as I did for apply_update_events().

wallet/src/wallet/mod.rs

notmandatory · 2025-10-30T20:56:18Z

For the branching question. I'd prefer to have a release/2.x branch that contains a tag for each published release. I don't think we'll need branches to do bug fixes separately for 2.1,2.2 etc.

Co-authored-by: Steve Myers <[email protected]>

tnull · 2025-10-31T12:19:34Z

I would like to see some tests. If you don't have time to do them I'm happy to push a commit with similar testing as I did for apply_update_events().

Yes, if you don't mind feel free to push a test case. I'm generally happy to do it, but given it will be the first one for apply_block API in general, you will have much more context to preset some test approach/design.

Also did minor cleanup of apply_update_events tests.

notmandatory · 2025-11-01T00:04:15Z

I added a few tests for apply_block_events based on the relevant (non-mempool related) tests I made for apply_update_events. I think these are the only ones needed for applying on-chain blocks, but feel free to modify these or add more.

ValuedMammal · 2025-11-02T00:18:33Z

For the branching question. I'd prefer to have a release/2.x branch that contains a tag for each published release. I don't think we'll need branches to do bug fixes separately for 2.1,2.2 etc.

That's a good suggestion.

ValuedMammal

ACK e9a3034

notmandatory

(self) ACK e9a3034

tnull requested a review from ValuedMammal as a code owner October 29, 2025 12:18

github-project-automation bot added this to BDK Wallet Oct 29, 2025

tnull mentioned this pull request Oct 29, 2025

Use BDK events in update_payment_store lightningdevkit/ldk-node#658

Open

ValuedMammal assigned tnull Oct 29, 2025

ValuedMammal moved this to In Progress in BDK Wallet Oct 29, 2025

ValuedMammal added this to the Wallet 3.0.0 milestone Oct 29, 2025

ValuedMammal added the new feature New feature or request label Oct 29, 2025

ValuedMammal modified the milestones: Wallet 3.0.0, Wallet 2.3.0 Oct 29, 2025

ValuedMammal requested a review from notmandatory October 29, 2025 16:32

Camillarhi reviewed Oct 29, 2025

View reviewed changes

notmandatory mentioned this pull request Oct 29, 2025

ci(msrv): pin syn crate to 2.0.106 #338

Merged

3 tasks

Add apply_block_events and apply_block_connected_to_events

df444d0

Previously, we added a new `Wallet::apply_update_events` method that returned `WalletEvent`s. Unfortunately, no corresponding APIs were added for the `apply_block` counterparts. Here we fix this omission.

tnull force-pushed the 2025-10-add-apply-block-events branch from 3039c1b to df444d0 Compare October 30, 2025 09:37

ValuedMammal changed the base branch from release/2.2 to release/2.3 October 30, 2025 18:52

notmandatory reviewed Oct 30, 2025

View reviewed changes

wallet/src/wallet/mod.rs Outdated Show resolved Hide resolved

thunderbiscuit mentioned this pull request Oct 30, 2025

Lining Up The Big 3.0 PRs #339

Open

f Duplicate event logic rather than business logic

3f0664e

Co-authored-by: Steve Myers <[email protected]>

test(wallet): add tests for apply_block_events

e9a3034

Also did minor cleanup of apply_update_events tests.

notmandatory mentioned this pull request Nov 1, 2025

Return wallet events when applying updates (3.0 milestone) #319

Draft

8 tasks

ValuedMammal changed the base branch from release/2.3 to release/2.x November 2, 2025 00:10

ValuedMammal moved this from In Progress to Needs Review in BDK Wallet Nov 4, 2025

ValuedMammal approved these changes Nov 4, 2025

View reviewed changes

notmandatory approved these changes Nov 5, 2025

View reviewed changes

notmandatory merged commit abc9cd8 into bitcoindevkit:release/2.x Nov 5, 2025
20 checks passed

github-project-automation bot moved this from Needs Review to Done in BDK Wallet Nov 5, 2025

ValuedMammal mentioned this pull request Nov 5, 2025

Release tracking wallet-2.3.0 #342

Open

3 tasks

Add apply_block_events and apply_block_connected_to_events #336

Add apply_block_events and apply_block_connected_to_events #336

Conversation

tnull commented Oct 29, 2025

Description

Notes to the reviewers

Checklists

All Submissions:

New Features:

Uh oh!

coveralls commented Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 18988009424

Details

💛 - Coveralls

Uh oh!

Camillarhi Oct 29, 2025

Choose a reason for hiding this comment

Uh oh!

tnull Oct 29, 2025

Choose a reason for hiding this comment

Uh oh!

ValuedMammal Oct 29, 2025

Choose a reason for hiding this comment

Uh oh!

tnull Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

notmandatory Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

notmandatory commented Oct 30, 2025

Uh oh!

tnull commented Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ValuedMammal commented Oct 30, 2025

Uh oh!

notmandatory left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

notmandatory commented Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tnull commented Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

notmandatory commented Nov 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ValuedMammal commented Nov 2, 2025

Uh oh!

ValuedMammal left a comment

Choose a reason for hiding this comment

Uh oh!

notmandatory left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Add `apply_block_events` and `apply_block_connected_to_events` #336

Add `apply_block_events` and `apply_block_connected_to_events` #336

coveralls commented Oct 29, 2025 •

edited

Loading

notmandatory Oct 30, 2025 •

edited

Loading

tnull commented Oct 30, 2025 •

edited

Loading

notmandatory commented Oct 30, 2025 •

edited

Loading

tnull commented Oct 31, 2025 •

edited

Loading

notmandatory commented Nov 1, 2025 •

edited

Loading